fix: Numeric value clearing in preset input controls#77139
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @DarkMatter-999! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Thanks for putting up a PR for this @DarkMatter-999 👍
The proposed fix addresses the original bug, but it introduces another very minor issue. It persists empty string values in the block attributes.
I think we can simplify the change here a little and both fix the bug and avoid persisting empty strings and unnecessary block attributes.
It would also help here to have some additional tests to protect against regressions here and confirm the fix.
Here's a proposed diff that would simplify the fix, protect against the empty string attribute value, and define some tests for the control
diff --git a/packages/block-editor/src/components/preset-input-control/index.js b/packages/block-editor/src/components/preset-input-control/index.js
index dbafef496b3..2d6df63f334 100644
--- a/packages/block-editor/src/components/preset-input-control/index.js
+++ b/packages/block-editor/src/components/preset-input-control/index.js
@@ -168,22 +168,20 @@ export default function PresetInputControl( {
unitConfig?.max ?? customValueSettings[ computedUnit ]?.max ?? 10;
const handleCustomValueChange = ( newValue ) => {
- const isExplicitlyEmpty = newValue === '';
- const isNumeric =
- newValue !== undefined && ! isNaN( parseFloat( newValue ) );
-
- let newCustomValue;
- if ( isExplicitlyEmpty ) {
- newCustomValue = '';
- } else if ( isNumeric ) {
- newCustomValue = newValue;
- } else {
- newCustomValue = undefined;
+ // Treat empty/undefined input as an explicit clear and propagate
+ // undefined so the attribute is removed rather than being persisted
+ // as an empty string.
+ if ( newValue === undefined || newValue === '' ) {
+ onChange( undefined );
+ return;
}
- if ( newCustomValue !== undefined ) {
- onChange( newCustomValue );
+ // Ignore non-numeric intermediate input (e.g. just a unit).
+ if ( isNaN( parseFloat( newValue ) ) ) {
+ return;
}
+
+ onChange( newValue );
};
const handleCustomValueSliderChange = ( next ) => {
onChange( [ next, computedUnit ].join( '' ) );
diff --git a/packages/block-editor/src/components/preset-input-control/test/index.js b/packages/block-editor/src/components/preset-input-control/test/index.js
index b46ca23d2e4..8252c727e5b 100644
--- a/packages/block-editor/src/components/preset-input-control/test/index.js
+++ b/packages/block-editor/src/components/preset-input-control/test/index.js
@@ -92,6 +92,76 @@ describe( 'PresetInputControl', () => {
expect( mockOnChange ).toHaveBeenCalledWith( '25px' );
} );
+ it( 'clears value with undefined when input is fully erased via backspace', () => {
+ render(
+ <PresetInputControl
+ { ...defaultProps }
+ presets={ presets }
+ value="60px"
+ disableCustomValues={ false }
+ />
+ );
+
+ const input = screen.getByRole( 'spinbutton' );
+
+ // Simulate the bug scenario: backspace through "60" one character
+ // at a time. fireEvent.change is used here (rather than userEvent
+ // keyboard interactions) because the controlled UnitControl input
+ // does not respond to synthesised key events in jsdom. The
+ // intermediate "6" forwarding is expected and correct; the bug
+ // was that the final clear silently failed to propagate, leaving
+ // the parent stuck on the partial value.
+ fireEvent.change( input, { target: { value: '6' } } );
+ fireEvent.change( input, { target: { value: '' } } );
+
+ // The final clear must propagate as undefined, not be swallowed,
+ // and never be persisted as an empty string.
+ expect( mockOnChange ).toHaveBeenLastCalledWith( undefined );
+ expect( mockOnChange ).not.toHaveBeenCalledWith( '' );
+ } );
+
+ it( 'clears value with undefined when input is cleared in one shot', async () => {
+ const user = userEvent.setup();
+
+ render(
+ <PresetInputControl
+ { ...defaultProps }
+ presets={ presets }
+ value="25px"
+ disableCustomValues={ false }
+ />
+ );
+
+ const input = screen.getByRole( 'spinbutton' );
+ await user.clear( input );
+
+ expect( mockOnChange ).toHaveBeenCalledWith( undefined );
+ expect( mockOnChange ).not.toHaveBeenCalledWith( '' );
+ } );
+
+ it( 'does not call onChange for non-numeric intermediate input', async () => {
+ const user = userEvent.setup();
+
+ render(
+ <PresetInputControl
+ { ...defaultProps }
+ presets={ presets }
+ value="15px"
+ disableCustomValues={ false }
+ />
+ );
+
+ const input = screen.getByRole( 'spinbutton' );
+ await user.clear( input );
+ mockOnChange.mockClear();
+
+ // Typing a stray non-numeric character should not propagate a change.
+ await user.type( input, 'a' );
+
+ expect( mockOnChange ).not.toHaveBeenCalledWith( 'a' );
+ expect( mockOnChange ).not.toHaveBeenCalledWith( 'apx' );
+ } );
+
it( 'uses select dropdown for many presets', async () => {
const manyPresets = Array.from( { length: 12 }, ( _, i ) => ( {
name: `Preset ${ i + 1 }`,
|
|
||
| let newCustomValue; | ||
| if ( isExplicitlyEmpty ) { | ||
| newCustomValue = ''; |
There was a problem hiding this comment.
Passing this through to onChange will result in empty string values in the block attributes persisted in the block comment delimiter. It would be best if we can fix this bug without doing this.
|
Thanks for the detailed review and for providing the test cases, @aaronrobertshaw ! I’ve updated the PR to use the simplified logic you suggested. I’ve also included the new tests to protect against future regressions. Ready for another look! |
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the quick iteration here @DarkMatter-999 👍
The simplified logic reads much more clearly and the new tests give us good coverage against regressions. Testing as advertised for me.
✅ Clearing a numeric input via backspace no longer leaves it stuck on the partial value
✅ Fully cleared input passes undefined so no empty string value is persisted
✅ Unit-only intermediate input value is ignored as expected
✅ New unit tests all passing locally
LGTM 🚢
What?
Closes #77124
Fixes a bug where clearing numeric values in shared preset-based controls (used by inputs like margin and border radius) could retain a partial value (for example,
6after deleting60).Why?
When users clear numeric values with backspace in affected block controls, the intermediate partial value may be applied and persisted instead of the field becoming fully empty. This creates incorrect style values and inconsistent UX.
How?
Updates
PresetInputControlcustom value change handling so empty input is treated as a valid cleared state instead of being discarded asundefined. This allows the control to propagate a true empty value and avoids persisting transient partial numeric states during deletion.Testing Instructions
Testing Instructions for Keyboard
60in controls backed by preset input (e.g. Margin, Padding).6is retained.Screenshots or screencast
Before
number-input.mov
After
number-input-fix.mov
Use of AI Tools
I used GitHub Copilot as an assistive drafting and code-review aid for the PR.